-
Notifications
You must be signed in to change notification settings - Fork 13
Add contextual error #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add contextual error #132
Conversation
|
@plebhash I’ve adapted the pool according to what we discussed. Let me know what you think. I’ll handle the JDC and Tproxy updates next in this PR. |
0c2036e to
a898fa1
Compare
looks good to me |
90d442c to
31504fc
Compare
GitGab19
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just started the review, but I prefer to have some replies from you before proceeding with the rest, to be sure I got the idea behind the Source introduced here.
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
|
Started a testing session with the changes of this PR. |
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
579ae19 to
7446ef0
Compare
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/extensions_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/jd_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/jd_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/jd_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/jd_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/jd_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/template_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/template_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/template_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/upstream_message_handler.rs
Outdated
Show resolved
Hide resolved
05c4386 to
72ff644
Compare
72ff644 to
f476065
Compare
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/downstream_message_handler.rs
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/upstream_message_handler.rs
Outdated
Show resolved
Hide resolved
miner-apps/jd-client/src/lib/channel_manager/upstream_message_handler.rs
Outdated
Show resolved
Hide resolved
| messages_results | ||
| .push(Err(JDCError::disconnect(e, *downstream_id))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be shutdown?
It seems an internal error
| return Err(JDCError::fallback( | ||
| JDCErrorKind::UpstreamMessageDuringSoloMining, | ||
| )); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we fallback while we're already doing SOLO mining?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gonna never happen, if it does, we just disconnect from the upstream entity. Should I just remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JDC sets up its mode based on SetupConnection.Success flag which is wrong, fix it.
miner-apps/translator/src/lib/sv2/channel_manager/channel_manager.rs
Outdated
Show resolved
Hide resolved
miner-apps/translator/src/lib/sv2/channel_manager/channel_manager.rs
Outdated
Show resolved
Hide resolved
miner-apps/translator/src/lib/sv2/channel_manager/mining_message_handler.rs
Outdated
Show resolved
Hide resolved
pool-apps/pool/src/lib/channel_manager/mining_message_handler.rs
Outdated
Show resolved
Hide resolved
pool-apps/pool/src/lib/channel_manager/mining_message_handler.rs
Outdated
Show resolved
Hide resolved
pool-apps/pool/src/lib/channel_manager/mining_message_handler.rs
Outdated
Show resolved
Hide resolved
pool-apps/pool/src/lib/channel_manager/mining_message_handler.rs
Outdated
Show resolved
Hide resolved
pool-apps/pool/src/lib/channel_manager/mining_message_handler.rs
Outdated
Show resolved
Hide resolved
pool-apps/pool/src/lib/channel_manager/mining_message_handler.rs
Outdated
Show resolved
Hide resolved
pool-apps/pool/src/lib/channel_manager/mining_message_handler.rs
Outdated
Show resolved
Hide resolved
|
@Shourya742 can you improve the commit history? |
|
A part from this, I want to report that I ran many manual tests locally, and I haven't noticed anything weird. So green light on my side, once the commits are better organized. |
98e1546 to
6fe1b5b
Compare
6fe1b5b to
eff4e66
Compare
GitGab19
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
I closed earlier PR by mistake and removed all the commits: #86.